-
Notifications
You must be signed in to change notification settings - Fork 310
local echo (7/7): Support simplified version of local echo #1453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
988c615
to
f297a65
Compare
aa81e2f
to
28d545f
Compare
I temporarily removed the last two commits because the confirmation dialog requires us to rework the implementation and tests more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Here's another round of review—it includes some comments I wrote yesterday on those commits that you temporarily removed in this revision, and they may or may not still apply to your new versions of those commits :)
lib/model/message_list.dart
Outdated
@@ -163,6 +191,7 @@ mixin _MessageSequence { | |||
/// Either the bottom slices of both [items] and [messages] are empty, | |||
/// or the first item in the bottom slice of [items] is a [MessageListMessageItem] | |||
/// for the first message in the bottom slice of [messages]. | |||
// TODO(#1453) update this in the context of outbox messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1453 is a PR number (this one) 🙂; did you mean to mention an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is just a TODO for myself to update this dart doc, since we can have outbox-message related items in the list.
lib/model/message_list.dart
Outdated
// This loop relies on the assumption that all items that follow | ||
// the last [MessageListMessageItem] are derived from outbox messages. | ||
// If there is no [MessageListMessageItem] at all, | ||
// this will end up removing end markers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the second sentence still true/necessary after c5e6957?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should end up removing all items, but that's less relevant now because we used to need to restore the end markers at the end. Updating it to describe the behavior accurately.
lib/model/message_list.dart
Outdated
// TODO insert in middle instead, when appropriate | ||
_addMessage(message); | ||
if (event.localMessageId != null) { | ||
final localMessageId = int.parse(event.localMessageId!); | ||
// [outboxMessages] is epxected to be short, so removing the corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "expected"
lib/widgets/message_list.dart
Outdated
child: Text(message.senderFullName, // TODO(#716): use `store.senderDisplayName` | ||
child: Text(message is Message | ||
? store.senderDisplayName(message as Message) | ||
: store.userDisplayName(message.senderId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some of this that can go in a separate commit? I notice it deletes a TODO for a separate issue.
lib/widgets/message_list.dart
Outdated
/// A placeholder for Zulip message sent by the self-user. | ||
/// | ||
/// See also [OutboxMessage]. | ||
class OutboxMessageWithPossibleSender extends StatelessWidget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about
/// A "local echo" placeholder for a Zulip message to be sent by the self-user.
so it doesn't look like it could apply to messages that were sent in the past.
lib/widgets/message_list.dart
Outdated
_SenderRow(message: message, showTimestamp: false), | ||
Padding( | ||
padding: const EdgeInsets.symmetric(horizontal: 16), | ||
// This is adapated from [MessageContent]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "adapted"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. Looks like I fixed in the wrong commit last time.
test/model/message_list_test.dart
Outdated
test('not in narrow', () => awaitFakeAsync((async) async { | ||
await prepare(narrow: eg.topicNarrow(stream.streamId, 'topic'), stream: stream); | ||
await prepareMessages(foundOldest: true, messages: | ||
List.generate(30, (i) => eg.streamMessage(stream: stream, topic: 'topic'))); | ||
await prepareOutboxMessages(count: 5, stream: stream); | ||
check(model).outboxMessages.isEmpty(); | ||
|
||
async.elapse(kLocalEchoDebounceDuration); | ||
checkNotNotified(); | ||
check(model).outboxMessages.isEmpty(); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prepareOutboxMessages
defaults to a topic 'some topic', which is indeed different from 'topic'. But that's not easy to see locally; how about passing an explicit value that's easy to compare with 'topic' to see that it's different.
lib/widgets/message_list.dart
Outdated
case OutboxMessageState.waitPeriodExpired: | ||
final isComposeBoxOffered = | ||
MessageListPage.ancestorOf(context).composeBoxState != null; | ||
handleTap = isComposeBoxOffered ? () => _handlePress(context) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: _handleTap
rather than _handlePress
, I think, for consistency with GestureDetector.onTap
Also, to be consistent with edit-message, let's apply the handler only to the message content, not also the sender row.
case OutboxMessageState.failed: | ||
case OutboxMessageState.waitPeriodExpired: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to show "MESSAGE NOT SENT" in the waitPeriodExpired
state? I think this could cause a confusing symptom like #1525, where the message has actually been sent (or will be) but we say it hasn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In v0.0.29 we'd enter this state even if we got a success response from the send request, if the event didn't then show up. I suspect that's what happened in the report in #1525.
In the current revision of this PR, a success response will put the outbox-message into waiting
state indefinitely, until the event arrives. I think showing this is OK if we've gotten neither the event nor a response from the request.
(The exact status in that case is more like "tried sending the message, it's been a while, haven't heard any response yet, so it may or may not have made it through, up to you if you want to retry". Not sure there's a good concise way to communicate that.)
lib/widgets/message_list.dart
Outdated
case OutboxMessageState.waiting: | ||
final designVariables = DesignVariables.of(context); | ||
return Padding( | ||
padding: const EdgeInsetsGeometry.only(top: 1.5, bottom: 0.5), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_OutboxMessageStatusRow
is similar enough to _EditMessageStatusRow
that I think it makes sense to be consistent between them in how we do this padding.
Experimentally, 0.5px on the bottom looks odd to me, as though it's meant to touch the bottom—and also the left and right edges of the screen—but it accidentally doesn't:
0.5px below | 4px below (edit-message) |
---|---|
![]() |
![]() |
I know your implementation follows the Figma, but in this case I would just do what we do for edit-message; the code is also simpler that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we have adjusted the debounce timeout in the more recent version #1472, I think the issue with the layout shift caused by the loading indicator still exists (#1453 (comment)), at least for messages that don't have fancy formatting.
We have 4px total in message content's bottom padding, and the height of the progress bar is 2px. How about moving the progress bar to the top half of the bottom padding, so that there is 2px instead of 0.5px after it? Meanwhile, we can adjust the bottom padding of the edit status row to be 2px so that they are consistent.'
2px below | 2px below (edit-message) |
---|---|
![]() |
![]() |
Note that the local echo indicator sits inside the bottom padding of the message content, and the one for edit-message doesn't. I think this is fine since there is label that doesn't fit within the padding for edit-message anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, SGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the review cycles with Chris aren't yet complete, but I wanted to take a look in so you have a review for tomorrow. I made my way through the first four commits after #1472 (which this PR is stacked atop a version of):
67e5e1f msglist: Remove trailingWhitespace
827939a test [nfc]: Extract {dm,stream}OutboxMessage helpers
363024a msglist test [nfc]: Make checkInvariant compatible with MessageBase
b0dc888 msglist [nfc]: Extract _addItemsForMessage
and looked at part of the main model commit:
5579e00 msglist: Add and manage outbox message objects in message list view
Generally this looks good; comments below, all more or less small. Probably the priority for tomorrow is those two final commits that were temporarily removed (#1453 (comment)).
lib/widgets/message_list.dart
Outdated
}, | ||
if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this feels like it makes things pretty crowded just above a recipient header.
msglist: Remove trailingWhitespace
This 11px whitespace can be traced back to 311d4d56e in 2022, and is not
present in the Figma design. It was there to match the web design while
prototyping the app.
I just spent a few minutes looking around in Figma and didn't find an example of a message list for an interleaved view, so one with messages followed by recipient headers. Do you know of one?
Probably it would be good to refine this spacing somehow, e.g. by skipping it when followed by the end of the feed rather than a recipient header. But let's not just take it out entirely. (And so for this PR, probably leave it the way it is, behavior-wise.)
It looks like in main it's already a constant. So the code could be simplified by propagating that constant down to this line, and therefore still removing the trailingWhitespace
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only ones I have seen are in design-process drafts, like this one. I will turn this into an NFC for this PR.
test/model/narrow_test.dart
Outdated
check(narrow.containsMessage( | ||
_TestDmMessage(allRecipientIds: [1, 2]))).isTrue(); | ||
eg.dmOutboxMessage(from: user1, to: [user2]))).isTrue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check's data is inconsistent: the narrow says selfUserId is 2, but the message says it's 1.
(I guess this was present in the version already in main and this change is NFC. But the from
/to
makes it more apparent than the allRecipientIds[0]
behavior did. That's a good thing about the new helpers. 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, right. The test was not set up correctly. Adding a prep commit (ideally we would change the old helpers to use from/to
, but I kept it minimal just so we can fix the test data), so that we can keep the refactor NFC.
test/model/message_list_test.dart
Outdated
if (message is Message) { | ||
check(model.store.messages)[message.id].isNotNull().identicalTo(message); | ||
} else if (message is OutboxMessage) { | ||
check(message).hidden.isFalse(); | ||
check(model.store.outboxMessages)[message.localMessageId].isNotNull().identicalTo(message); | ||
} else { | ||
assert(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This first check can become two separate loops, making the logic of both of them simpler.
(Arguably it should have been a separate loop in the first place — it's checking something logically fairly different from what the rest of this loop body is checking. But that had less impact when it was one line.)
@@ -2151,17 +2684,27 @@ void checkInvariants(MessageListView model) { | |||
check(model).fetchOlderCoolingDown.isFalse(); | |||
} | |||
|
|||
for (final message in model.messages) { | |||
check(model.store.messages)[message.id].isNotNull().identicalTo(message); | |||
final allMessages = [...model.messages, ...model.outboxMessages]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through the existing checks on model.messages
, here's another that should gain a counterpart for model.outboxMessages
:
check(isSortedWithoutDuplicates(model.messages.map((m) => m.id).toList()))
.isTrue();
The outbox messages should be ordered by localMessageId.
lib/model/message_list.dart
Outdated
if (!messagesSameDay(prevMessageItem.message, message)) { | ||
items.add(MessageListDateSeparatorItem(message)); | ||
canShareSender = false; | ||
} else { | ||
canShareSender = (prevMessageItem.message.senderId == message.senderId); | ||
canShareSender = prevMessage.senderId == message.senderId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: keep this using prevMessageItem.message
rather than prevMessage
The previous item is the thing that directly matters for whether this next item should share its sender row. It's also what's used a few lines above in the messagesSameDay
call, so that keeps the logic a bit more internally coherent to read.
/// | ||
/// Usually this should not have that many items, so we do not anticipate | ||
/// performance issues with unoptimized O(N) iterations through this list. | ||
final List<OutboxMessage> outboxMessages = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds some overhead in magnitude of O(1) (where the constant is
the number of outbox messages in a view, expected to be small) on
message event handling.
That is not what the word "constant" means 🙂
Instead can say:
This adds some overhead on message-event handling, linear in the number of outbox messages in a view. We rely on that number being small.
Or could say something like "O(k), where there are k outbox messages in the view".
lib/model/message_list.dart
Outdated
required super.showSender, | ||
required super.isLastInBlock, | ||
}) : content = ZulipContent(nodes: [ | ||
ParagraphNode(links: [], nodes: [TextNode(message.contentMarkdown)]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParagraphNode(links: [], nodes: [TextNode(message.contentMarkdown)]), | |
ParagraphNode(links: null, nodes: [TextNode(message.contentMarkdown)]), |
See the doc on the links
field:
/// A list of all [LinkNode] descendants.
///
/// An empty list is represented as null.
/// […]
@@ -718,7 +718,11 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat | |||
return MessageItem( | |||
key: ValueKey(data.message.id), | |||
header: header, | |||
trailingWhitespace: 11, | |||
item: data); | |||
case MessageListOutboxMessageItem(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implements minimal support to display outbox message message item
widgets in the message list, without indicators for theirs states.
Retrieving content from failed sent requests and the full UI are
implemented in a later commit.
Is there — ah, I guess that was in the commits you temporarily removed as mentioned at #1453 (comment) .
This 11px whitespace can be traced back to 311d4d5 in 2022. While this is not present in the Figma design, it would be a good idea to refine it in the future. See discussion: zulip#1453 (comment)
This changes the bottom padding from 4px to 2px, to match the later added status row for outbox messages. See discussion: zulip#1453 (comment)
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441
This changes the bottom padding from 4px to 2px, to match the later added status row for outbox messages. See discussion: zulip#1453 (comment)
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441
Thanks for the reviews! Pushed an update, this time with the commits I removed last time, with a bunch of prep commits. I also went through this PR's interaction with |
Some visual changes in this update: screenshotsThe error message for discarding draft on restoring a message not sent is tentative:
![]() The bottom padding of the status row has changed:
from #1453 (comment) |
Thanks for the revision! The changes related to my review at #1453 (review) all look good, and the initial 5 commits all look good to me now: I haven't yet read the subsequent commits in full: @chrisbobbe will be taking over this PR; I'll leave those latter commits until after he's made a pass through them. |
Great! Merging those first five, with a small commit-message tweak:
|
This 11px whitespace can be traced back to 311d4d5 in 2022. While this is not present in the Figma design, it would be a good idea to refine it in the future. See discussion: #1453 (comment)
This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment)
This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment) [chris: fixed to maintain 4px bottom padding in the common case where the progress bar is absent] Co-authored-by: Chris Bobbe <[email protected]>
And I've sent revised versions of the next chunk, in #1535. |
This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment) [chris: fixed to maintain 4px bottom padding in the common case where the progress bar is absent] Co-authored-by: Chris Bobbe <[email protected]>
This adds some overhead on message-event handling, linear in the number of outbox messages in a view. We rely on that number being small. We add outboxMessages as a list independent from messages on _MessageSequence. Because outbox messages are not rendered (the raw content is shown as plain text), we leave the 1-1 relationship between `messages` and `contents` unchanged. When computing `items`, we now start to look at `outboxMessages` as well, with the guarantee that the items related to outbox messages always come after those for other messages. Look for places that call `_processOutboxMessage(int index)` for references, and the changes to `checkInvariants` on how this affects the message list invariants. `addOutboxMessage` is similar to `handleMessage`. However, outbox messages do not rely on the fetched state, i.e. they can be synchronously updated when the message list view was first initialized. This implements minimal support to display outbox message message item widgets in the message list, without indicators for theirs states. Retrieving content from failed sent requests and the full UI are implemented in a later commit. Co-authored-by: Chris Bobbe <[email protected]>
Issue zulip#720 is superseded by zulip#1441, in which we'll still clear the compose box when the send button is tapped. (We'll still preserve the composing progress in case the send fails, but we'll do so in an OutboxMessage instead of within the compose input in a disabled state.)
Issue zulip#720 is superseded by zulip#1441, and these don't apply... I guess with the exception of a note on how a generic "x" button could be laid out, so we leave that.
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
OK, and now rebased with a revision ready for review! This is stacked atop #1538, which is a separate PR because it's a substantial commit. |
Fixes #1441.
Stacked atop #1472.
(This branch can be used to preview the full implementation)
screenshots